-
-
Notifications
You must be signed in to change notification settings - Fork 493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better cover the regex related rules. #608
Conversation
3781c68
to
e1ffa59
Compare
e1ffa59
to
490b775
Compare
@@ -31,13 +31,6 @@ class WordPress_Sniffs_PHP_DiscouragedFunctionsSniff extends Generic_Sniffs_PHP_ | |||
* @var array(string => string|null) | |||
*/ | |||
public $forbiddenFunctions = array( | |||
// Deprecated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe leave a comment that this is handled with the new sniff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a comment like that in the code will become very untidy in combination with the upcoming #582 work. I'd rather suggest at some point also re-ordering the vip
ruleset according to their documentation and making a note in the ruleset about rules like this which are already covered in core
. Ok ?
Is there no way that we can just have the PHPCompatibility sniff library as a dependency? Maybe it is best to just copy the sniff over, but I shy away from anything that might require routine maintenance. 😄 |
See #575 as well where I originally suggested that. The only way I can think of would be to make it a dependency via composer and add it to the ruleset. Not sure how well that would work for the re-hashing to get PHPCS to recognize the installed standards, but I'd leave that up to @Rarst to advise on. This is not fool-proof no matter what. Not everyone uses Composer, so this could mean people will start complaining about WPCS not working as it breaks on the missing ruleset.
As for now, for the handbook coverage, we'd only need the one sniff, that seemed like the simplest way to handle it. If/when WPCS would want to start incorporating more sniffs from PHPCompatibility, then this should be revisited IMHO.. |
OK, sounds good to me. |
490b775
to
41d4cf8
Compare
Rebased for merge conflicts. |
41d4cf8
to
3cc54fe
Compare
I am concerned about licensing here, that library is LGPL. By copy/pasting pieces of it here (into MIT project) we are making a potential mess of that. |
Good point. Would it help if we had formal permission of the project owner ? |
Not really, it's just a... GPL mess in such case. Using it as a library would be clear separation of stuff, which LGPL allows without issue. |
094de1b
to
b2aad8b
Compare
FYI: Working on an alternative solution for the |
* Removes similar checks from existing sniffs (PHP.DiscouragedFunctions and VIP.RestrictedFunctions). * Makes sure that *all* functions in the POSIX extension are covered by this check - they previously were not. * Add unit tests for the new sniff covering *all* POSIX functions.
b2aad8b
to
7721698
Compare
What with the upcoming release and the work @grappler is doing in #633, I'm removing the commit which added the I'm working on an abstract class to detect regular expressions and concrete classes for the |
@jrfnl Just to confirm—this is ready for merge? |
@JDGrimes Absolutely, it now only contains the splitting off of the POSIX check from the VIP sniff to an individual sniff and adding that rule to the The other part of the regex rules will be covered in a separate PR. |
To be clear phpcombatibility got thrown out of this, so I can stop worry about licensing stuff? ;) |
@Rarst You can (for now) ;-) |
All and @Rarst in particular: I've been doing quite some work on the PHPCompatibility sniff library in the mean time and have created a number of utility functions which would also be useful for WPCS and could get rid of a sh*tload of semi-duplicate code and allow us to check certain things with higher precision. As I created these functions - though sometimes based on existing code - and contributed them to PHPCompatibility, will it be ok license-wise if I port them over to WPCS ? At a later time - once these functions have been used a bit more and gone through the wrangler, I'd even like to PR them to PHPCS itself. |
If by "based on existing code" you mean copy/paste/modify of its code then it's a GPL derivative work and ugh ugh ugh mess for us. I am not familiar with specific case of LGPL into GPL move to say confidently about it. As above it would be clearly fine if LGPL lib was kept as dependency, but I don't really know about mixing their code. |
By "based on existing code", I mean inspired by. The actual code was rewritten. All the same, I find this whole licensing thing very confusing. If I write code and choose to contribute the same code to three different projects, it now sounds like the licensing of the first project to merge the code would rule over it all. That just feels wrong and discourages from contributing at all. I also wonder if code snippets like that can be licensed or even copyrighted at all, after all, writing it from scratch someone else will just end up with 90% the same code with maybe some minor personalizations. Being able to re-use bits and pieces will encourage best of breed coding much more than everyone re-inventing the wheel. On a side-note: I remember seeing somewhere a note in the WPCS code base about a piece of code which was taken from WP Core. As WP Core is GPL, according to the licensing, that code shouldn't have been allowed in in that case. Correct ? |
If you wrote the code from scratch then it's fine. It's your work to be licensed as you see fit.
Nope. Since it's your code you would be free to contribute it to three different project and license it differently for each one. After the code is contributed to the project it's subject to its license, but you as copyright holder of your code are free to license it differently for another project.
Yes, it's vague and often people close eyes at technicalities. My completely personal rule of a thumb — copy/paste of actual code is a line behind which it becomes a clear unarguable derivative code. Many people (WordPress, cough) would disagree though and draw a line differently.
Correct. If it's still the case we should get rid of it. |
@Rarst Thanks for your detailed reply! Will keep this in mind 👍 |
So we find ways to not do something rather then ways to do something? What about dual licensing code sniffer to make it easier to expand it? |
Welcome to GPL.
Dual licensing makes things harder, not easier. Now you have two licenses to be compatible with at the same time. |
Actually no :) Dual licensing means either one applies, from everything that I have read. The user would pick the one to use. In the case of GPLv2 or later versus MIT, WordPress would then pick GPLv2 or later. |
Yes, user can pick which license to apply, good for them. And user can do that because project is obliged to have project's code comply with both. Think about it. I don't like GPL? I put GPL into dual license project and say it's now MIT... Doesn't work. |
So what code in Code Sniffer is MIT specific? |
All of it, as long as that's the license project declares. I am not too keen to continue general licensing chatter in this ticket. If you have specific licensing concerns or proposals for the project please open a dedicated issue. |
Nah I won't pursue it but thanks for asking :) |
From the handbook:
Ref: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#regular-expressions
This can be split into three distinct rules:
POSIX
functions/e
modifierRule 1 was up to now not covered in core. Parts of it were covered in
extra
and invip
, but the implementations were inconsistent and both were incomplete.Rule 2 was not covered at all.
Rule 3 is already covered by the single vs double quotes sniff.
This PR fixes the issues with covering rule 1 and 2 with the following two commits:
Split off
POSIX
Functions restrictions to its own sniff.PHP.DiscouragedFunctions
andVIP.RestrictedFunctions
).POSIX
extension are covered by this check - they previously were not.POSIX
functions.Copy in the
PregReplaceEModifierSniff
from thePHPCompatibility
Sniff library.PHPCompatibility
folder to theSniff
folder.PregReplaceEModifierSniff
.PHPCompatibility_Sniff
.PHPCompatibility
sniff folder about its usage.PHPCompatibility
folder from the PHPCS code style checks - this way we can just drop-in replace the file (ok, except for the class renaming) when there is a new version available.